Skip to content

Extend plot_csd support to SEEG, ECoG, and DBS channel types#13713

Open
Aniketsy wants to merge 13 commits intomne-tools:mainfrom
Aniketsy:fix-13711
Open

Extend plot_csd support to SEEG, ECoG, and DBS channel types#13713
Aniketsy wants to merge 13 commits intomne-tools:mainfrom
Aniketsy:fix-13711

Conversation

@Aniketsy
Copy link
Contributor

@Aniketsy Aniketsy commented Mar 2, 2026

Fixes #13711

@Aniketsy Aniketsy requested a review from drammock as a code owner March 2, 2026 12:57
@Aniketsy Aniketsy marked this pull request as draft March 2, 2026 13:22
Copy link
Contributor

@tsbinns tsbinns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this, definitely on the right track! Some comments below.

@Aniketsy
Copy link
Contributor Author

Aniketsy commented Mar 3, 2026

@tsbinns thanks for the review and for suggesting this approach, it makes much more sense than my earlier implementation. please have a look and let me know if any further improvements are needed.

Copy link
Contributor

@tsbinns tsbinns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good changes! A couple small comments on the code, and a slightly bigger one for to refine the test. Let me know if anything's unclear.

@tsbinns tsbinns marked this pull request as ready for review March 4, 2026 19:37
@tsbinns
Copy link
Contributor

tsbinns commented Mar 4, 2026

@larsoner @wmvanvliet I noticed that the existing behaviour when plotting is that if no valid channel types are present (currently only EEG, Mag, Grad; this PR would change it to all valid data types), it just returns an empty figs container list.

I was a bit surprised by this. My expectation was at least a warning would be given that there is nothing valid to plot. WDYT?

Aniketsy and others added 3 commits March 5, 2026 13:53
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Copy link
Contributor

@tsbinns tsbinns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress! A couple minor comments to tidy things up.

Aniketsy and others added 5 commits March 6, 2026 09:59
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
@tsbinns
Copy link
Contributor

tsbinns commented Mar 7, 2026

@Aniketsy I'd say this is looking good, but I'd still like to get this comment addressed (#13713 (comment)) and maybe also a review from @wmvanvliet.

Please also add your name to doc/changes/names.inc and use the :newcontrib: role on your name in the changelog (no underscore is required).

@Aniketsy
Copy link
Contributor Author

Aniketsy commented Mar 7, 2026

ERROR: Could not find a version that satisfies the requirement statsmodels>=0.15.0.dev697 (from versions: none)
ERROR: No matching distribution found for statsmodels>=0.15.0.dev697

I noticed this ci failure in two PR's @tsbinns should we relax the statsmodel version requirement?

@tsbinns
Copy link
Contributor

tsbinns commented Mar 9, 2026

That was addressed in #13728. Don't worry about merging the updated branch until there's new code to push. Will save us some cost on running CI jobs.

@Aniketsy
Copy link
Contributor Author

Aniketsy commented Mar 9, 2026

Just saw it. Makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expand channel types supported for CSD plotting

2 participants